Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr/21 commit2 #26

Draft
wants to merge 10 commits into
base: devel
Choose a base branch
from
Draft

Pr/21 commit2 #26

wants to merge 10 commits into from

Conversation

rossctaylor
Copy link
Contributor

Reworked the console output to have basic logging functionality which can be redirected to any ostream.

@rossctaylor rossctaylor requested review from dlazenby and rarmstro May 19, 2021 00:03
@rossctaylor
Copy link
Contributor Author

@dlazenby - I basically added some simple logging capability which defaults to cout and cerr. For your purposes, you'd call log.EnableFormatting(false) to disable the formatting and then you'd specify a sink (ostream) for each logging level. Does this seem reasonable?

@dlazenby
Copy link

Unfortunately it's a bad time for me, I'm out on field assignment the next two weeks so I have limited time to review. I looked over these changes and it seems reasonable to me. A lot more thorough than what I had done. I didn't want to add another dependency to quanergy's driver code, but spdlog is a good open source library that implements a sink model. This reminds me of that library.

I will say that I had trouble with running multiple drivers (client connections) simultaneously using the logging code that I had written. It would cause intermittent thread conflicts. Please test running 2 or more client connections to multiple sensors from a single application to make sure that logging stream is threadsafe. It is normal for us to have 5+ sensors all connected to the same application.

@rossctaylor rossctaylor self-assigned this May 20, 2021
@rossctaylor rossctaylor marked this pull request as draft May 20, 2021 15:06
@rossctaylor
Copy link
Contributor Author

I may actually go back to the signal/slot idea do to flexibility. I started to use this in the ros node and it is kind of a pain to have it use ostream...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants